Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve CentralDogmaPropertySupplierIntegrationTest #197

Closed
wants to merge 1 commit into from

Conversation

thachlp
Copy link
Member

@thachlp thachlp commented Mar 29, 2023

Related issue: #187
As Javadoc of CentralDogmaRule class we should create rules by

@ClassRule
public static final CentralDogmaRule rule = new CentralDogmaRule();

The problem with @Rule is it will start and stop an embedded server for each test, I think it may affect performance tests and can cause the EndpointSelectionTimeoutException random fail.

@CLAassistant
Copy link

CLAassistant commented Mar 29, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

If we share single CentralDogmaRule instance among tests, doesn't it cause potential race between tests? (e.g. testA create file => testB depends on file's absence)

We should avoid bring such dependencies in tests because it makes tests fragile, so we should fix the root cause of EndpointSelectionTimeoutException rather than mitigating the chance by reducing instance creation.

@thachlp
Copy link
Member Author

thachlp commented Mar 31, 2023

Thanks for the patch!

If we share single CentralDogmaRule instance among tests, doesn't it cause potential race between tests? (e.g. testA create file => testB depends on file's absence)

We should avoid bring such dependencies in tests because it makes tests fragile, so we should fix the root cause of EndpointSelectionTimeoutException rather than mitigating the chance by reducing instance creation.

@ocadaruma Thanks for replying to me 😊
As I understand, delcaton uses CentralDogma to store properties, so the test should be the same as the use-case behavior, we start only one centraldogma server, and working on this... I also referenced to centraldogma code to see how they test.

From the issue log, I see it is a random fail, so I just think about the problem, we start/stop the dogma server for each test, so we can not sure if the server is already for access or not (even it started success)

@thachlp thachlp requested a review from ocadaruma March 31, 2023 10:00
Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

H-mm, maybe I still don't quite understand well why this patch is supposed to fix the flaky test.

I understand making CentralDogmaRule as ClassRule can reduce the chance of the failure though, still the root cause is not fixed so same failure could happen, right?

so the test should be the same as the use-case behavior

Current patch could fail depending on the test execution order. (why currently it succeeds is just "lucky").

In testFileExist, it creates a file in the CD project. On the other hand, testFileNonExistent checks the absence of the file.
So if these tests are executed in testFileExist => testFileNonExistent, testFileNonExistent will fail.
(Though JUnit4 determines the test execution order deterministically, we shouldn't write a test that could fail depending on the execution order in general)

So, to get fresh CD instances every time, using @Rule is more simple.

@thachlp
Copy link
Member Author

thachlp commented Apr 1, 2023

H-mm, maybe I still don't quite understand well why this patch is supposed to fix the flaky test.

I understand making CentralDogmaRule as ClassRule can reduce the chance of the failure though, still the root cause is not fixed so same failure could happen, right?

so the test should be the same as the use-case behavior

Current patch could fail depending on the test execution order. (why currently it succeeds is just "lucky").

In testFileExist, it creates a file in the CD project. On the other hand, testFileNonExistent checks the absence of the file. So if these tests are executed in testFileExist => testFileNonExistent, testFileNonExistent will fail. (Though JUnit4 determines the test execution order deterministically, we shouldn't write a test that could fail depending on the execution order in general)

So, to get fresh CD instances every time, using @Rule is more simple.

Thank you for clear explanation to me @ocadaruma, so I will close this pr and work on another pr (if I can find the root cause), again, thank you

@thachlp thachlp closed this Apr 1, 2023
@thachlp thachlp deleted the issue-187 branch February 26, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants